use maybe_async that supports async traits for breakpoint #161

SidongYang · 2025-01-02T02:29:08Z

Description

This PR is exprimental codes that uses maybe_async for supporting fully async traits. Adding simple maybe_async macro for trait and it generates async code when is_sync feature is disabled. For this there is new feature is_sync for gdbstub which is default feature. So it doesn't affacts original sync code.

issue: #159

API Stability

This PR does not require a breaking API change

Checklist

Documentation
- Ensured any public-facing rustdoc formatting looks good (via cargo doc)
- (if appropriate) Added feature to "Debugging Features" in README.md
Validation
- Included output of running examples/armv4t with RUST_LOG=trace + any relevant GDB output under the "Validation" section below
- Included output of running ./example_no_std/check_size.sh before/after changes under the "Validation" section below
If implementing a new protocol extension IDET
- Included a basic sample implementation in examples/armv4t
- IDET can be optimized out (confirmed via ./example_no_std/check_size.sh)
- OR implementation requires introducing non-optional binary bloat (please elaborate under "Description")
If upstreaming an Arch implementation
- I have tested this code in my project, and to the best of my knowledge, it is working as intended.

Validation

Supports async and no change for sync codegen

daniel5151

Thanks for sending in this PR!

At first blush, maybe-async certainly seems to tick many of the boxes I'm interested in, particularly regarding having a single unified codebase for both sync and async code.

There are, however, a few things that concern me...

Given that async-trait requires boxing futures... I would like to better understand and profile how that extra indirection affects the dead-code-elimination guarantees that the IDET pattern otherwise guarantees in release mode.

At minimum, as a next-step in this exploration: can you sprinkle some maybe-async annotations into the armv4t code, such that we can do some side-by-side comparisons between the sync and async APIs via the in-tree /scripts/test_dead_code_elim.sh script?

Alternatively - it seems that maybe_async(AFIT) exists... but using it would be a bit annoying, and when gdbstub is configured in async mode, all current IDET methods would need to return impl Trait instead of dyn Trait, since Rust doesn't have native dyn async traits yet. Maybe that sort of transformation could be done automatically by forking / extending maybe_async... unlear.

Oh, and of course - boxing futures isn't particularly no_std friendly... and it would certainly be interesting to have gdbstub's async APIs play nice with no_std projects using async infrastructure (e.g: via embassy). I wouldn't say that "perfect" async no_std support is a hard requirement for any v0 of async gdbstub... but I would like to at least understand what sorts of gaps / implications will result once we introduce async stuff into the gdbstub codebase.

P.S: don't worry about those clippy lints. If you rebase your PR on top of latest master, I've gone ahead and fixed those.

daniel5151 · 2025-01-05T21:32:29Z

Cargo.toml

@@ -35,6 +37,7 @@ std = ["alloc"]
 trace-pkt = ["alloc"]
 paranoid_unsafe = []
 core_error = []
+sync = ["maybe-async/is_sync"]


Whether or not gdbstub should present a sync vs. async API by default is an interesting question. I suppose that's a judgement call we can make later, if we decide that maybe-async is the basis of the approach we take.

daniel5151 · 2025-01-05T21:38:43Z

src/stub/mod.rs

@@ -154,7 +155,8 @@ impl<'a, T: Target, C: Connection> GdbStub<'a, T, C> {
    /// etc...) you will need to interface with the underlying
    /// [`GdbStubStateMachine`](state_machine::GdbStubStateMachine) API
    /// directly.
-    pub fn run_blocking<E>(
+    #[maybe_async]
+    pub async fn run_blocking<E>(


I know this is just experimental code... but having a run_blocking method that is actually async seems funky. I suppose we'll have to think a bit harder about how to model / rename this API if we go down this route

Agreed, It needs to be renamed

SidongYang · 2025-01-07T08:31:06Z

It seems that CI failed because in compiling no std example, it cannot find Box for maybe_async macro.

SidongYang · 2025-01-07T08:49:04Z

@daniel5151 I don't want to break default features for compatibility. but I can't find good way to choose sync traits as default.
It's little tricky that sync feature is default. User who wants async will disable default features.

daniel5151 · 2025-01-08T18:30:18Z

Thanks for iterating here!

Now that the armv4t example is working alongside async, lets run some of those experiments I mentioned above, regarding dead code elimination. i.e: comment out the supports_breakpoints (and/or its sub-IDETs), and double-check that the breakpoint code does indeed still get eliminated in the async case.

There are other, arguably more interesting experiments that I'd like to run before committing to this approach. Namely: I'd like to explore the use of maybe_async(AFIT), and whether dropping dyn Target support when using async is a viable approach. The allocations per handler method make me a bit queasy, given the otherwise aggressively allocation-optimized API of gdbstub as a whole...

More ambitiously, as I mull things over even more, I'm wondering if it might make sense to just go all-in on async APIs without relying on maybe_async, and manually using AFITs, and maybe using some blanket impls / helper structs / helper traits to support sync targets? I'm not certain if the Rust compiler will even support that sort of thing, so getting a POC working in the rust playground might be worthwhile before trying to bring the technique back into the main gdbtsub repo.

All that is to say - while it's really neat to see that maybe_async does seem to work in theory, I'm not sure I can commit to this approach until we've nailed down answers to some of these other design considerations that gdbstub adheres to.

Let me know if you think you can help dig into some of these experiments here. If not, I'll try to allocate some time at some point in the next few weeks to see if I can play around here. There are a few different gdbstub related things that I'd like to hack on, and I'm starting to feel like I might be close to the tipping point where enough things have piled up that I'll need to allocate some time towards actively developing / refactoring gdbstub myself, vs. purely acting as a shephard for other folks' PRs 😅

@daniel5151 I don't want to break default features for compatibility. but I can't find good way to choose sync traits as default. It's little tricky that sync feature is default. User who wants async will disable default features.

Indeed, I think there's no good way to get around this issue when using maybe-async. It would be nice if maybe-async had more flexibility here, wrt. having a sync_by_default + is_async set of feature flags. I wouldn't call it a blocker or anything... but yeah, unfortunate.

SidongYang · 2025-01-13T11:45:55Z

I agree that AFIT options is best for using maybe_async. I pushed a commit that use maybe_async(AFIT)
And I've checked the deadcode elimination test script. I can't pass the test without any commenting out. but it's same as master branch. and it passed the test with returning None in support_sw_breakpoint.
And I want to work for this in this PR. But if there is better way to support async, It's okay to close this PR. You can do some experiments for this issue also it would be thanks to share that.

SidongYang mentioned this pull request Jan 2, 2025

Async traits #159

Open

daniel5151 reviewed Jan 5, 2025

View reviewed changes

SidongYang force-pushed the feature/use-maybe-async branch 3 times, most recently from b81abcd to 70063c9 Compare January 6, 2025 03:45

SidongYang added 4 commits January 6, 2025 03:47

use maybe_async that supports async traits for breakpoint

d9a0fdd

impl async hw breakpoint, singlethread resume

e7d638b

use sync as default feature

b5b6a36

use maybe_async on examples

a91ab2e

SidongYang force-pushed the feature/use-maybe-async branch from 70063c9 to a91ab2e Compare January 6, 2025 03:48

use sync for no std example

326b98e

use AFIT for maybe_async

9fea11d

SidongYang force-pushed the feature/use-maybe-async branch from 32613d3 to 9fea11d Compare January 13, 2025 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use maybe_async that supports async traits for breakpoint #161

use maybe_async that supports async traits for breakpoint #161

SidongYang commented Jan 2, 2025 •

edited

Loading

daniel5151 left a comment

daniel5151 Jan 5, 2025

daniel5151 Jan 5, 2025

SidongYang Jan 7, 2025

SidongYang commented Jan 7, 2025

SidongYang commented Jan 7, 2025

daniel5151 commented Jan 8, 2025

SidongYang commented Jan 13, 2025

use maybe_async that supports async traits for breakpoint #161

Are you sure you want to change the base?

use maybe_async that supports async traits for breakpoint #161

Conversation

SidongYang commented Jan 2, 2025 • edited Loading

Description

API Stability

Checklist

Validation

daniel5151 left a comment

Choose a reason for hiding this comment

daniel5151 Jan 5, 2025

Choose a reason for hiding this comment

daniel5151 Jan 5, 2025

Choose a reason for hiding this comment

SidongYang Jan 7, 2025

Choose a reason for hiding this comment

SidongYang commented Jan 7, 2025

SidongYang commented Jan 7, 2025

daniel5151 commented Jan 8, 2025

SidongYang commented Jan 13, 2025

SidongYang commented Jan 2, 2025 •

edited

Loading